Skip to content

[RFC] feat(linux): s2idle: Document the mode selection logic#641

Draft
DhruvaG2000 wants to merge 1 commit intoTexasInstruments:masterfrom
DhruvaG2000:s2idle_mode_sel_v1
Draft

[RFC] feat(linux): s2idle: Document the mode selection logic#641
DhruvaG2000 wants to merge 1 commit intoTexasInstruments:masterfrom
DhruvaG2000:s2idle_mode_sel_v1

Conversation

@DhruvaG2000
Copy link
Copy Markdown
Collaborator

Document the mode selection logic using the s2idle flow

Copy link
Copy Markdown
Contributor

@kwillis01 kwillis01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be good to see if there are other places in the previous parts of the doc that can be updated or that the new info can loop into and consolidate.

Comment thread source/linux/Foundational_Components/Power_Management/pm_psci_s2idle.rst Outdated
Comment thread source/linux/Foundational_Components/Power_Management/pm_psci_s2idle.rst Outdated
Comment thread source/linux/Foundational_Components/Power_Management/pm_psci_s2idle.rst Outdated
Comment thread source/linux/Foundational_Components/Power_Management/pm_psci_s2idle.rst Outdated
Comment thread source/linux/Foundational_Components/Power_Management/pm_psci_s2idle.rst Outdated
Copy link
Copy Markdown
Contributor

@kwillis01 kwillis01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly looks good, I think that the QoS Latency Constraints and Mode Selection section needs to be fixed up to flow better and be a little more condensed.

Comment thread source/linux/Foundational_Components/Power_Management/pm_psci_s2idle.rst Outdated

**Setting QoS Constraints from User Space:**

Applications can constrain the system's low-power behavior by writing to the PM QoS device file.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be specified that it is the PM QoS CPU wakeup latency file. Just because this sentence makes it sound like each device would have a PM QoS file, when its only for CPUs.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ack

3. Only idle states with ``exit-latency-us`` ≤ constraint are considered
4. The deepest eligible state is selected

**Setting QoS Constraints from User Space:**
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this section should be combined with the Example: Deep Sleep Mode Selection section. You're showing an example of how to set the QoS constraint without properly explaining it. Also by combining with the Example: Deep Sleep Mode Selection section you can make this into an example for just deepsleep

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okayy

that the cpuidle governor correctly respects QoS constraints and selects the appropriate idle state
based on latency requirements.

**Selecting Specific Low-Power Modes:**
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This section should go after the How It Sets QoS Constraints: section since that is when the /dev/cpu_wakeup_latency is explained

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@kwillis01
Copy link
Copy Markdown
Contributor

Additionally, could you update the RTC + DDR section in pm_am62lx_low_power_modes.rst with the updated commands needed to enter RTC + DDR? Then you can point to this page from there if the user wants an indepth explanation on s2idle

Document the mode selection logic using the s2idle flow

Signed-off-by: Dhruva Gole <d-gole@ti.com>
@github-actions
Copy link
Copy Markdown

New warnings found with rstcheck:

source/linux/Foundational_Components/Power_Management/pm_psci_s2idle.rst:537: (ERROR/3) Content block expected for the "note" directive; none found.
source/linux/Foundational_Components/Power_Management/pm_psci_s2idle.rst:538: (WARNING/2) Explicit markup ends without a blank line; unexpected unindent.

Comment on lines +537 to +539
.. note::
For complete Device Tree definitions including all latency parameters, refer to the platform's
device tree source files (e.g., ``k3-am62l3-evm-idle-states.dtso``).
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
.. note::
For complete Device Tree definitions including all latency parameters, refer to the platform's
device tree source files (e.g., ``k3-am62l3-evm-idle-states.dtso``).
.. note::
For complete Device Tree definitions including all latency parameters, refer to the platform's
device tree source files (e.g., ``k3-am62l3-evm-idle-states.dtso``).


.. code-block:: bash

exec 4<>/dev/cpu_wakeup_latency; echo 0x3e8 >&4
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems a little convoluted for what could just be

Suggested change
exec 4<>/dev/cpu_wakeup_latency; echo 0x3e8 >&4
echo 0x3e8 | tee /dev/cpu_wakeup_latency

or

Suggested change
exec 4<>/dev/cpu_wakeup_latency; echo 0x3e8 >&4
echo 0x3e8 > /dev/cpu_wakeup_latency

Like I know that exec line is a more literal interpretation of the above c example, but if someone's doing it through bash then they probably don't really understand those redirects.

Comment on lines +697 to +704
printf("QoS set to %s. Press Ctrl+C to exit.\n", LATENCY_VAL);

while (keep_running)
sleep(1);

close(fd);
printf("Released.\n");
return 0;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we holding this process open with an aggressive loop? There's nothing in the kernel docs that says the value is dropped when the FD is released.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants